🥧[WEB-4164] - Adding Pricing Card CTA Arrow Icon #582
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces an Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
aralovelace
force-pushed
the
WEB-4164
branch
from
4573989 to
927d515
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/core/Button.tsx (1)
Line range hint
98-112: Button component is not passing iconColor to commonButtonInterior.The
Buttoncomponent destructures and passes various props but misses theiconColorprop, which means icon colors won't work in the base Button component.const Button: React.FC<PropsWithChildren<ButtonProps>> = ({ variant = "primary", size, leftIcon, rightIcon, children, className, + iconColor, ...rest }) => { return ( <button {...commonButtonProps({ variant, size, leftIcon, rightIcon, className })} {...(rest as React.ButtonHTMLAttributes<HTMLButtonElement>)} > - {commonButtonInterior({ leftIcon, rightIcon, children })} + {commonButtonInterior({ leftIcon, rightIcon, iconColor, children })} </button> ); };
🧹 Nitpick comments (2)
src/core/Button.tsx (1)
92-94: Consider using undefined instead of empty string for additionalCSS.Using an empty string for
additionalCSSwheniconColoris not provided might not be the best approach. Consider usingundefinedinstead, which better represents the absence of a value.- {leftIcon ? <Icon name={leftIcon} additionalCSS={iconColor || ''} /> : null} + {leftIcon ? <Icon name={leftIcon} additionalCSS={iconColor} /> : null} - {rightIcon ? <Icon name={rightIcon} additionalCSS={iconColor || ''} /> : null} + {rightIcon ? <Icon name={rightIcon} additionalCSS={iconColor} /> : null}src/core/Pricing/data.tsx (1)
Line range hint
171-175: Consider adding iconColor to Enterprise plan CTAThe Enterprise plan's CTA is missing the iconColor property while all other plans have it. Consider adding it using the orange theme to maintain consistency with the Enterprise plan's color scheme.
cta: { text: "Contact us", url: "/contact", + iconColor: "text-orange-600 dark:text-orange-600", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/Button.tsx(3 hunks)src/core/LinkButton.tsx(3 hunks)src/core/Pricing/PricingCards.tsx(1 hunks)src/core/Pricing/data.tsx(3 hunks)src/core/Pricing/types.ts(1 hunks)
🔇 Additional comments (5)
src/core/Pricing/types.ts (1)
17-17: LGTM! Type definition is well-structured.
The iconColor property is correctly typed as optional and uses the appropriate color types (ColorClass | ColorThemeSet).
src/core/Button.tsx (1)
32-35: LGTM! Property documentation is clear.
The JSDoc comments clearly document the purpose of the iconColor property.
src/core/LinkButton.tsx (1)
8-8: LGTM! Implementation is correct and consistent.
The LinkButton component correctly:
- Imports the necessary color types
- Adds the
iconColorproperty to its props - Destructures and passes the property to
commonButtonInterior
Also applies to: 13-13, 25-25, 52-52
src/core/Pricing/data.tsx (1)
17-21: LGTM! Consistent color theming across pricing tiers
The iconColor values are well-aligned with each plan's visual hierarchy:
- Free plan uses neutral colors
- Standard/Pro plans use blue to indicate premium status
Also applies to: 76-76, 128-132
src/core/Pricing/PricingCards.tsx (1)
195-196: LGTM! Clean implementation of icon customization
The addition of rightIcon and iconColor props is well-implemented:
- Properly passes the iconColor from the cta object
- Consistently uses the arrow icon across all CTAs
aralovelace
force-pushed
the
WEB-4164
branch
from
927d515 to
15c234e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/Button.tsx (2)
89-94: Consider renamingadditionalCSSto better reflect its purpose.While the implementation is correct, the prop name
additionalCSSon the Icon component might be misleading since we're specifically passing a color value. Consider renaming it to something more specific likecolorClassorthemeColorto better reflect its purpose.
Line range hint
1-117: Document the new iconColor feature in component documentation.Since this is a core UI component, consider updating the component's documentation (e.g., Storybook stories, README, or design system documentation) to showcase the new
iconColorfeature and provide usage examples.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/Button.tsx(4 hunks)src/core/LinkButton.tsx(3 hunks)src/core/Pricing/PricingCards.tsx(1 hunks)src/core/Pricing/data.tsx(3 hunks)src/core/Pricing/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/Pricing/types.ts
- src/core/LinkButton.tsx
- src/core/Pricing/PricingCards.tsx
- src/core/Pricing/data.tsx
🔇 Additional comments (2)
src/core/Button.tsx (2)
5-5: LGTM! Well-documented type definition.
The new iconColor property is properly typed and documented. Making it optional maintains backward compatibility.
Also applies to: 32-35
106-106: Verify the impact on existing button styles.
The implementation correctly forwards the iconColor prop to commonButtonInterior. However, let's verify that this change doesn't affect existing button styles where iconColor isn't specified.
Also applies to: 114-114
✅ Verification successful
No impact on existing button styles - implementation is safe
The iconColor prop is correctly implemented and won't affect existing button styles because:
- It's properly forwarded through commonButtonInterior to the Icon component as additionalCSS
- Existing button usage in the codebase shows iconColor is already being used in multiple places (PricingCards, ProductTile) with specific color values
- When iconColor is not specified, the Icon component will render without the additionalCSS prop, maintaining the default styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing button usage to ensure backward compatibility
rg -t tsx -t jsx "Button.*>|LinkButton.*>" --no-heading
Length of output: 85
Script:
#!/bin/bash
# Search for Button component usage patterns
rg --type-add 'ts:*.{ts,tsx}' --type-add 'jsx:*.jsx' -t ts -t jsx "<Button|<LinkButton" -A 2
# Check the Button component implementation
ast-grep --pattern 'const Button = ({ $$$props$$$ }: ButtonProps) => {
$$$
}'
# Check if iconColor has a default value or is optional in the type definition
ast-grep --pattern 'interface ButtonProps {
$$$
iconColor?: $_
$$$
}'
Length of output: 2045
Script:
#!/bin/bash
# Check ButtonProps interface definition
ast-grep --pattern 'interface ButtonProps {
$$$
}'
# Check commonButtonInterior implementation
ast-grep --pattern 'const commonButtonInterior = ($$$) => {
$$$
}'
# Look for any existing iconColor usage
rg --type-add 'ts:*.{ts,tsx}' -t ts 'iconColor.*=|iconColor:' -B 1 -A 1
Length of output: 2703
jamiehenson
force-pushed
the
WEB-4164
branch
from
b5a5304 to
b5638ad
Compare
Jira Ticket Link / Motivation
WEB-4164 Add IconColor for the LinkButton and add rightIcon on all Pricing Cards CTA
Summary of changes
This pull request introduces several changes to the
Button,LinkButton, andPricingcomponents to support customizable icon colors. The most important changes include adding a newiconColorprop to these components and updating their usage in relevant files.Button and LinkButton components:
src/core/Button.tsx: AddediconColorprop toButtonPropsBaseand updated thecommonButtonInteriorandButtoncomponents to use this new prop. [1] [2] [3] [4]src/core/LinkButton.tsx: AddediconColorprop toLinkButtonPropsand updated theLinkButtoncomponent to use this new prop. [1] [2] [3]Pricing component:
src/core/Pricing/PricingCards.tsx: UpdatedPricingCardscomponent to passiconColorprop toLinkButton.src/core/Pricing/data.tsx: AddediconColorto thectaobjects inplanData. [1] [2] [3]src/core/Pricing/types.ts: UpdatedPricingDataFeatureCtatype to include theiconColorprop.How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
iconColorproperty for buttons and links, allowing customization of icon colors.PricingCardsto utilize the newiconColorproperty for call-to-action buttons.Bug Fixes
Documentation
iconColorproperty in relevant components.